Bring targets out of alerts #1353
Conversation
Adds CRUD operations for targets TODO- - Storage of targets depending upon RBAC discussion - Modify alerts to use targets using ID (AlertConfig update)
WalkthroughThis update introduces a new asynchronous, ID-based system for managing alert targets. Targets are now referenced by unique ULIDs instead of embedded objects, with a global concurrent registry for CRUD operations. New HTTP endpoints enable RESTful management of targets with RBAC authorization. Persistent storage integration supports loading and saving target configurations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP_Handler as HTTP Handler (/targets)
participant TARGETS as TargetConfigs (Registry)
participant Storage
Client->>HTTP_Handler: POST /targets (Target JSON)
HTTP_Handler->>TARGETS: update(target)
TARGETS->>Storage: Save target to persistent storage
HTTP_Handler-->>Client: Target created (JSON)
Client->>HTTP_Handler: GET /targets
HTTP_Handler->>TARGETS: list()
TARGETS->>Storage: (on load) get_targets()
HTTP_Handler-->>Client: List of targets (JSON)
Client->>HTTP_Handler: GET /targets/{id}
HTTP_Handler->>TARGETS: get_target_by_id(id)
HTTP_Handler-->>Client: Target (JSON)
sequenceDiagram
participant AlertHandler
participant TARGETS as TargetConfigs
AlertHandler->>TARGETS: get_target_by_id(target_id)
TARGETS-->>AlertHandler: Target object (or error)
AlertHandler->>Target: validate() / trigger_notifications()
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ Context from checks skipped due to timeout of 90000ms (10)
🔇 Additional comments (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Alerts creation will require the ULID of the target instead of the target body
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/handlers/http/modal/server.rs (1)
258-275: Consider using target-specific RBAC actions for clarity.The targets webscope implementation is well-structured with proper RESTful endpoints. However, using alert-related actions (
GetAlert,PutAlert,DeleteAlert) for target operations might be confusing from a permissions perspective.Consider introducing target-specific RBAC actions like
GetTarget,PutTarget,DeleteTargetfor better clarity and separation of concerns, unless the current approach is intentional due to the close relationship between alerts and targets.src/storage/object_storage.rs (1)
998-1002: Address the TODO for distributed mode consistency.The TODO comment indicates this needs to be updated for distributed mode, similar to
alert_json_path. Consider tracking this technical debt to ensure both functions are updated together when implementing distributed mode support.Would you like me to create an issue to track updating both
alert_json_pathandtarget_json_pathfor distributed mode?src/handlers/http/targets.rs (2)
39-39: Remove or update the misleading comment.The comment "add to the map" doesn't make sense for a list operation.
- // add to the map
65-65: Fix typo in comment.- // esnure that the supplied target id is assigned to the target config + // ensure that the supplied target id is assigned to the target config
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/alerts/mod.rs(7 hunks)src/alerts/target.rs(4 hunks)src/handlers/http/alerts.rs(1 hunks)src/handlers/http/mod.rs(1 hunks)src/handlers/http/modal/query_server.rs(1 hunks)src/handlers/http/modal/server.rs(3 hunks)src/handlers/http/targets.rs(1 hunks)src/storage/mod.rs(1 hunks)src/storage/object_storage.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
src/handlers/http/alerts.rs (1)
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1346
File: src/handlers/http/health_check.rs:81-90
Timestamp: 2025-06-16T02:04:58.990Z
Learning: In the shutdown function in src/handlers/http/health_check.rs, the design approach is to log errors from sync operations rather than propagate them. This is intentional because the shutdown function is called on SIGTERM/SIGINT signals, and the goal is to perform best-effort cleanup (syncing pending files to object storage) while allowing the shutdown to proceed regardless of sync failures. Logging provides debugging information without blocking the shutdown process.
src/handlers/http/modal/server.rs (1)
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:0-0
Timestamp: 2025-05-01T10:27:56.858Z
Learning: The `add_tile()` function in `src/handlers/http/users/dashboards.rs` should use `get_dashboard_by_user(dashboard_id, &user_id)` instead of `get_dashboard(dashboard_id)` to ensure proper authorization checks when modifying a dashboard.
src/storage/object_storage.rs (2)
Learnt from: de-sh
PR: parseablehq/parseable#1185
File: src/handlers/http/logstream.rs:255-261
Timestamp: 2025-02-14T09:49:25.818Z
Learning: In Parseable's logstream handlers, stream existence checks must be performed for both query and standalone modes. The pattern `!PARSEABLE.streams.contains(&stream_name) && (PARSEABLE.options.mode != Mode::Query || !PARSEABLE.create_stream_and_schema_from_storage(&stream_name).await?)` ensures proper error handling in both modes.
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1263
File: src/handlers/http/ingest.rs:300-310
Timestamp: 2025-03-26T06:44:53.362Z
Learning: In Parseable, every stream is always associated with a log_source - no stream can exist without a log_source. For otel-traces and otel-metrics, strict restrictions are implemented where ingestion is rejected if a stream already has a different log_source format. However, regular logs from multiple log_sources can coexist in a single stream.
🧬 Code Graph Analysis (2)
src/handlers/http/modal/query_server.rs (1)
src/handlers/http/modal/server.rs (1)
get_targets_webscope(258-275)
src/alerts/target.rs (9)
src/storage/mod.rs (2)
new(183-185)new(196-202)src/alerts/mod.rs (7)
new(113-119)new(154-166)new(177-183)load(847-870)update(902-904)delete(944-951)validate(579-632)src/handlers/http/modal/mod.rs (1)
new(256-278)src/handlers/http/targets.rs (3)
update(55-80)list(38-43)delete(83-92)src/handlers/http/logstream.rs (2)
list(82-102)delete(47-80)src/parseable/streams.rs (1)
v(611-613)src/handlers/http/modal/query/querier_logstream.rs (1)
delete(52-110)src/handlers/http/modal/ingest/ingestor_logstream.rs (1)
delete(60-79)src/alerts/alerts_utils.rs (1)
value(413-413)
🔇 Additional comments (8)
src/storage/mod.rs (1)
62-62: LGTM! Consistent with existing patterns.The new
TARGETS_ROOT_DIRECTORYconstant follows the established naming convention and placement pattern used by other metadata directory constants in this module.src/handlers/http/mod.rs (1)
50-50: LGTM! Proper module integration.The targets module declaration follows the established pattern and maintains alphabetical ordering with other HTTP handler modules.
src/handlers/http/modal/query_server.rs (1)
72-72: LGTM! Proper service integration.The targets webscope is correctly registered in the QueryServer route configuration, following the same pattern as other service registrations.
src/handlers/http/alerts.rs (1)
51-51: Verify async conversion of AlertRequest → AlertConfig across the codebaseOur search for other
AlertRequest.into().await?orAlertConfig::from(AlertRequest)patterns didn’t surface any additional occurrences. To be safe:• Manually scan for any other
AlertRequest→AlertConfigconversions (e.g. viarg "AlertRequest" && rg "AlertConfig") and update them to the new async form if needed.
• Ensure all callers nowawaitthe conversion and properly handle errors.
• Benchmark the alert handler under realistic loads to catch any unexpected latency introduced by the async lookup.src/handlers/http/modal/server.rs (2)
29-29: LGTM! Proper module import.The targets module import is correctly placed with other handler imports.
91-91: LGTM! Consistent service registration.The targets webscope service registration follows the established pattern and is properly integrated with other services.
src/storage/object_storage.rs (1)
427-444: LGTM! Consistent implementation with get_alerts().The implementation follows the same pattern as the existing
get_alerts()method, which is good for consistency.src/alerts/target.rs (1)
44-99: Well-structured concurrent target management.The
TargetConfigsimplementation properly handles concurrent access with async RwLock and provides a clean API for target management.
Signed-off-by: parmesant <[email protected]>
parmesant
force-pushed
the
targets-separation
branch
from
329b787 to
cd363c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/alerts/target.rs (1)
128-141: Implementation differs from past review suggestion.A past review suggested implementing HTTP health checks for target validation, but this implementation checks for duplicates instead. The current approach is valid but differs from the original suggestion.
The duplicate checking logic addresses a different concern than target liveness validation.
🧹 Nitpick comments (1)
src/alerts/target.rs (1)
55-64: Consider error handling for storage operations.The
get_targets()call usesunwrap_or_default()which silently ignores storage errors. Consider logging storage failures for debugging purposes.- for alert in store.get_targets().await.unwrap_or_default() { + let targets = match store.get_targets().await { + Ok(targets) => targets, + Err(e) => { + warn!("Failed to load targets from storage: {}", e); + Vec::new() + } + }; + for alert in targets {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/alerts/mod.rs(7 hunks)src/alerts/target.rs(4 hunks)src/handlers/http/targets.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/handlers/http/targets.rs
- src/alerts/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: coverage
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
🔇 Additional comments (4)
src/alerts/target.rs (4)
30-31: LGTM: Appropriate imports added.The
itertools::Itertoolsandonce_cell::sync::Lazyimports are correctly added to support the new functionality.
44-46: Global static is well-implemented.The lazy initialization with
RwLock<HashMap<Ulid, Target>>provides thread-safe access with good read performance for the target registry.
123-124: Well-implemented ID field addition.The
idfield withUlid::newdefault provides unique identification for targets. The serde configuration correctly handles serialization.
276-277: Consistent ID handling in TargetVerifier.The ID field is properly added to
TargetVerifierand correctly transferred during conversion toTarget.Also applies to: 310-310
|
Need to update Quest to reflect the changes. Will send a PR there as well |
parmesant
force-pushed
the
targets-separation
branch
2 times, most recently
from
c511001 to
9ccb849
Compare
parmesant
force-pushed
the
targets-separation
branch
from
e6fe12f to
cd39b98
Compare
Adds CRUD operations for targets
TODO-
Fixes #XXXX.
Description
This PR has:
Summary by CodeRabbit
New Features
Bug Fixes
Chores